Remove wasm-support-plugin and deal with the code #515
Conversation
|
I don't have a strong preference either way -- if this PR gets merged I'll close mine. If you expect to add more instructions over time (for example when adding support for SIMD or multi-memory) then keeping the table might be better. Java is not generally considered a terse language, so having a bit of codegen can be useful for making the programmer's intent clear.
I was definitely surprised to see TSV used here. Similar tables I've seen in other projects treat tabs and spaces as just plain whitespace, which tends to survive editor round-trips better. For an example see the Linux kernel's Regardless of which approach is used, thank you for taking the time. I'll answer your questions in the other PR in case that helps with the decision. |
| for (OpCode e : OpCode.values()) { | ||
| byOpCode.put(e.opcode(), e); | ||
| } | ||
| signature.put(UNREACHABLE, new WasmEncoding[] {}); |
There was a problem hiding this comment.
Since each opcode has an encoding signature, we can simplify this and make it less error prone by moving the signature to the enum constructor (can static import the encoding constants):
UNREACHABLE(0x00, List.of()),
NOP(0x01, List.of()),
BLOCK(0x02, List.of(VARUINT)),
LOOP(0x03, List.of(VARUINT)),
BR_TABLE(0x0E, List.of(VEC_VARUINT, VARUINT)),The constructor call would be simpler if we use varargs for the encodings, but being explicit about the signature seems better.
| return byOpCode.get(opcode); | ||
| } | ||
|
|
||
| private static final Map<OpCode, WasmEncoding[]> signature = new HashMap<>(); |
There was a problem hiding this comment.
Let's change this to Map<OpCode, List<WasmEncoding>> so that it is immutable
|
|
||
| static { | ||
| for (OpCode e : OpCode.values()) { | ||
| byOpCode.put(e.opcode(), e); |
There was a problem hiding this comment.
Might want a validation here
if (byOpCode.put(e.opcode(), e) != null) {
throw new AssertionError("duplicate opcode: " + e.opcode());
}There was a problem hiding this comment.
Moved the Map to be an array, I see the value of the check, but is, ultimately, only for us developers of the engine.
I left it out for now.
There was a problem hiding this comment.
Agreed, and you would find the error immediately anyway, so it's not needed.
andreaTP
force-pushed
the
rem-wasm-support-plugin
branch
from
0edb29d to
a8f3ffc
Compare
|
@electrum moved to a fully hand rolled implementation, let me know what you think about this! |
There was a problem hiding this comment.
Looks great!
I don't know if an array is needed here. With 8 byte references, each array is 512kB, so this adds a 1MB memory overhead. Does this provide a measurable improvement when parsing WASM files?
If there is no improvement and the memory overhead is a problem for someone, we can switch back to a hash map later.
| // System.out.println("b: " + b + " op: " + op); | ||
| var signature = OpCode.getSignature(op); | ||
| var signature = OpCode.signature(op); | ||
| // TODO: Encode this in instructions.tsv ? |
andreaTP
force-pushed
the
rem-wasm-support-plugin
branch
from
a8f3ffc to
3527e6e
Compare
3 participants
Alternative ( to #514 ) fix for #513
I think this is simpler and, at the end of the day, more easily maintainable (fun fact: I have seen multiple ppl stumbling on the
.tsvas IntelliJ was not respecting the tabs 😅 ).